Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crash on delete DataSet #1531

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Conversation

kswenson
Copy link
Member

@kswenson kswenson commented Oct 2, 2024

There were several things going on here:

  • The primary cause of the hang/crash was an infinite loop in the GraphDataConfigurationModel's attempt to synchronize its filteredCases array after its DataSet was destroyed.

Once that was fixed, there were still several clients that were attempting to access the defunct DataSet triggering warnings. These have also been fixed:

  • The FilteredCases class now responds to disposal of its DataSet, thus preventing further access.
  • The CaseTile's HideShowMenuList checks whether its DataSet is alive before accessing it. (We conventionally avoid isAlive in models, but in components there often isn't a better way.)
  • The CountAdornmentComponent was properly using mstAutorun, but the model it was attached to was the CountAdornmentModel, not the axis models that were also accessed within the autorun.
  • Therefore, mstAutorun and mstReaction have been extended to support an array of model dependencies or a single model and so now the CountAdornmentComponent can specify all of its model dependencies. (I have wondered for some time whether we would ever encounter a situation in which a single model dependency was insufficient for these utilities and now we have.)

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.09%. Comparing base (b49838e) to head (7cfddb4).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1531      +/-   ##
==========================================
- Coverage   85.17%   85.09%   -0.08%     
==========================================
  Files         562      563       +1     
  Lines       28074    28157      +83     
  Branches     7722     7240     -482     
==========================================
+ Hits        23911    23961      +50     
- Misses       3860     4040     +180     
+ Partials      303      156     -147     
Flag Coverage Δ
cypress 74.39% <93.75%> (-0.07%) ⬇️
jest 53.34% <94.44%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

cypress bot commented Oct 2, 2024

codap-v3    Run #4528

Run Properties:  status check passed Passed #4528  •  git commit b76f82aad1: fix: crash on delete DataSet (#1531)
Project codap-v3
Branch Review main
Run status status check passed Passed #4528
Run duration 09m 40s
Commit git commit b76f82aad1: fix: crash on delete DataSet (#1531)
Committer Kirk Swenson
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 30
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 227
View all changes introduced in this branch ↗︎

@kswenson kswenson marked this pull request as ready for review October 2, 2024 20:01
@kswenson kswenson requested a review from bfinzer October 2, 2024 20:01
@kswenson kswenson added the v3 CODAP v3 label Oct 2, 2024
Copy link
Contributor

@bfinzer bfinzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻LGTM

@kswenson kswenson merged commit b76f82a into main Oct 3, 2024
11 checks passed
@kswenson kswenson deleted the 188236957-fix-delete-data-set branch October 3, 2024 20:24
lublagg pushed a commit that referenced this pull request Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 CODAP v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants